Gsi fed #590
Gsi fed #590
Conversation
|
@xyzemc and @guoqing-noaa , could you please review this PR? |
| ! prgmmr: j guo <jguo@nasa.gov> | ||
| ! org: NASA/GSFC, Global Modeling and Assimilation Office, 610.3 | ||
| ! date: 2018-08-10 | ||
| ! |
There was a problem hiding this comment.
Line 3-8: Is Guo involved in the development of this gsi_fedOper.f90 file? If not, suggest removing these lines.
There was a problem hiding this comment.
Not involved. I will update the documentation accordingly.
src/gsi/gsi_fedOper.F90
Outdated
|
|
||
| ! headNode => obsLList_headNode(self%obsLL(ibin)) | ||
| ! call intjo(headNode, rval,sval) | ||
| ! headNode => null() |
There was a problem hiding this comment.
Do we need line 151-153? If not, suggest removing them
There was a problem hiding this comment.
We don't currently need this code. I agree that it is good to remove these lines.
src/gsi/gsi_fedOper.F90
Outdated
|
|
||
| ! headNode => obsLList_headNode(self%obsLL(ibin)) | ||
| ! call stpjo(headNode,dval,xval,pbcjo(:),sges,nstep) | ||
| ! headNode => null() |
There was a problem hiding this comment.
Line 180-182: Similar as above, suggest removing them if not needed.
src/gsi/m_fedNode.F90
Outdated
| public:: fedNode | ||
|
|
||
| type,extends(obsNode):: fedNode | ||
| !type(td_ob_type),pointer :: llpoint => NULL() |
There was a problem hiding this comment.
Line 39: remove if not needed
There was a problem hiding this comment.
Agreed. I'm removing this commented out variable, plus several other similar commented out lines that were lingering from an earlier version of the code.
src/gsi/m_fedNode.F90
Outdated
| ! procedure, nopass:: headerRead => obsHeader_read_ | ||
| ! procedure, nopass:: headerWrite => obsHeader_write_ | ||
| ! procedure:: init => obsNode_init_ | ||
| ! procedure:: clean => obsNode_clean_ |
There was a problem hiding this comment.
Line 56, 58, 59, 68-71: remove if not needed
src/gsi/m_fedNode.F90
Outdated
| character(len=*),parameter:: MYNAME="m_fedNode" | ||
|
|
||
| !#define CHECKSUM_VERBOSE | ||
| !#define DEBUG_TRACE |
There was a problem hiding this comment.
Line 84-85: remove if not needed
src/gsi/m_fedNode.F90
Outdated
| type is(fedNode) | ||
| ptr_ => aNode | ||
| ! class default | ||
| ! call die(myname_,'unexpected type, aNode%mytype() =',aNode%mytype()) |
There was a problem hiding this comment.
Line 101-102: remove if not needed
src/gsi/m_fedNode.F90
Outdated
| if(.not.associated(aNode)) return | ||
| select type(aNode) | ||
| type is(fedNode) | ||
| ptr_ => aNode |
There was a problem hiding this comment.
As suggested, I improved the indentation.
src/gsi/read_fed.f90
Outdated
| ! | ||
| ! | ||
| DO i=1,numfed | ||
| DO k=1,maxlvl |
There was a problem hiding this comment.
Should change 'DO' to lower case to be consistent with 'end do' at the end of loop?
src/gsi/read_fed.f90
Outdated
|
|
||
| write(6,*) ' ------- check max and min value of OBS: bufr fed -------' | ||
| write(6,*) ' level maxval(fed) minval(fed)' | ||
| DO k=1,maxlvl |
There was a problem hiding this comment.
Same here, change the 'DO' to lower case 'do' ?
src/gsi/read_fed.f90
Outdated
| fed_max=-999.99 | ||
| ndata2=0 | ||
| DO i=1,numfed | ||
| DO k=1,maxlvl |
There was a problem hiding this comment.
Same here, change the 'DO' to lower case 'do' for Line 395 & 396?
src/gsi/setupfed.f90
Outdated
| real(r_kind),dimension(nobs) :: FEDMdiag,FEDMdiagTL | ||
| real(r_kind),dimension(nobs) :: FEDMdiag2D | ||
| integer(i_kind) :: npt | ||
| integer(i_kind) :: nobsfed |
There was a problem hiding this comment.
delete the space before :: to align the line above
src/gsi/setupfed.f90
Outdated
| do ifld=2,nfldsig | ||
| call gsi_bundlegetpointer(gsi_metguess_bundle(ifld),trim(varname),rank3,istatus) | ||
| ges_q(:,:,:,ifld)=rank3 | ||
| enddo |
There was a problem hiding this comment.
Changed "enddo" to "end do" in 13 locations.
src/gsi/setupfed.f90
Outdated
| else | ||
| write(6,*) trim(myname),': ', trim(varname), ' not found in met bundle, ier= ',istatus | ||
| call stop2(999) | ||
| endif |
There was a problem hiding this comment.
Change "endif" to "end if" in 31 locations.
| enddo | ||
| endif | ||
|
|
||
| end subroutine contents_binary_diag_ |
There was a problem hiding this comment.
there are too many 'endif' 'enddo' that I think is not consistent to GSI's format, please correct all of them in this subroutine.
| endif | ||
|
|
||
| end subroutine contents_binary_diag_ | ||
| subroutine contents_netcdf_diag_(odiag) |
There was a problem hiding this comment.
there are too many 'endif' 'enddo' that I think is not consistent to GSI's format, please correct all of them in this subroutine too.
src/gsi/statsconv.f90
Outdated
| if(nread > 0)then | ||
| if(first)then | ||
| open(iout_fed) | ||
| else |
There was a problem hiding this comment.
correct the indent before 'else'
|
@daviddowellNOAA Could you please update this PR? The due date of this PR is 8/24/2023. |
|
@xyzemc could you please review David's change? |
|
@xyzemc and @guoqing-noaa Thanks for the reviews. I made all the changes you suggested. Following the code changes, I compiled GSI on Jet and ran the Observer with conventional, radar-reflectivity, and FED observations. The GSI Observer produces identical results to what it did previously. That is the expected result. @ShunLiu-NOAA The PR is now updated. |
|
@ShunLiu-NOAA I don't know how to do a regression test, so I'll need some help. |
|
@daviddowellNOAA please check the instruction here: https://github.com/NOAA-EMC/GSI/wiki/GSI-Ctests-(regression-tests) |
|
Thanks @ShunLiu-NOAA! I'll attempt a regression test today and report results. |
|
Here are the results of the regression tests. The number of failures is discouraging. What's the next step in the process? 1/9 Test #1: [=[global_3dvar]=] ...............***Failed 120.45 sec 56% tests passed, 4 tests failed out of 9 Total Test time (real) = 3921.11 sec The following tests FAILED: |
|
@daviddowellNOAA Please point me the location of the your regression test cases. There were many reasons that could cause failure. Some of them may not be from the new code. |
|
Thanks @hu5970. I ran the regression tests here on Hera: /scratch2/BMC/wrfruc/dowell/GSI |
|
For the first 2 cases The reason of crash is because you do not have permission to read observation files:
So, I (or someone else) have to run those two regression test suites for you. The "netcdf_fv3_regional" failed because of memory check. This is not a critical failure: The global_enkf need more investigation. The control run failed because of the namelist: Did you change any namelist option for this test? Thanks, |
|
@hu5970 Thanks for the investigation! For the global_enkf test, for some reason the regression_namelists.sh in my branch differed in two ways from the regression_namelists.sh in develop. When I make the namelists consistent, then my branch passes the global_enkf test. 1/1 Test #9: [=[global_enkf]=] ................ Passed 2484.58 sec 100% tests passed, 0 tests failed out of 1 Total Test time (real) = 2484.60 sec |
|
@hu5970 @daviddowellNOAA |
|
@guoqing-noaa Works for me. We still have the issue of being unable to run the global_3dvar and global_4denvar regression tests owing to file permissions. What's our next step @hu5970 @ShunLiu-NOAA ? |
|
@daviddowellNOAA You cannot access those files because you are not in the "rstprod" group (restricted data). |
|
@daviddowellNOAA I can run the regression test for you. Let me know if there are any special things I need to pay attention to. |
|
Thanks @guoqing-noaa . The only trick with my branch seems to be the regression/regression_namelists.sh file. Two changes are needed: (1) Remove use_qsatensmean namelist parameter, i.e., change line 2172 from (2) Change pseudo_rh from .true. to .false. on line 2158. |
|
Awesome. Thanks @guoqing-noaa . I'm taking notes so I'll know how to do this next time. |
|
The regression test results are at: For the failed netcdf_fv3_regional case, it is because it failed the scalability test. |
|
Thanks @guoqing-noaa! Here's what I got for the netcdf_fv3_regional regression test with the latest code: The runtime for netcdf_fv3_regional_loproc_updat is 31.956025 seconds and is within the maximum allowable operational time of 1200 seconds, The runtime for netcdf_fv3_regional_loproc_updat is 31.956025 seconds and is within the allowable threshold time of 38.683795 seconds, The runtime for netcdf_fv3_regional_hiproc_updat is 30.413437 seconds and is within the allowable threshold time of 36.823723 seconds, The memory for netcdf_fv3_regional_loproc_updat is 35247860 KBs. This has exceeded maximum allowable hardware memory limit of 2516582 KBs, The memory for netcdf_fv3_regional_loproc_updat is 35247860 KBs and is within the maximum allowable memory of 38772839 KBs, The results (penalty) between the two runs (netcdf_fv3_regional_loproc_updat and netcdf_fv3_regional_loproc_contrl) are reproducible. The results (penalty) between the two runs (netcdf_fv3_regional_loproc_updat and netcdf_fv3_regional_hiproc_updat) are reproducible The case has passed the scalability regression test. It seems results for this regression test are not reproducible. |
|
I also tested the code, after the sync with the latest develop, for the GSI observer test case (with conventional, radar-reflectivity, and FED observations) on Jet. It continues to produce identical results to previous tests. |
|
@daviddowellNOAA You are right. The @ShunLiu-NOAA I would think this PR is ready to be merged. Thanks! |
|
I did regression test for this PR on WCOSS2: The reason for RTMA failure is: |
|
Revert to the careful work @MichaelLueken did as GSI code manager. Built GSI_FED and develop with Debug compilation flagged numerous
remark GSI Coding Standards require removal of unused variables in newly added source code
The debug builds of |
|
Looks like many of the |
|
I will work with David to clean those unused variables and make a clean branch from current head of the develop branch. |
Thank you @hu5970 . Not all the unused variables can be removed at present. Some are unused because they are placeholders for future development. |
|
Debug tests The debug global_4denvar global_3dvar hwrf_nmm_d3 rtma It is likely that additional errors exist in the code. One would need to modify the code to address the above errors, recompile, and rerun. This cycle would likely need to be repeated several times before all tests ran to completion in debug mode. The errors above are not unique to Absent a GSI code manager is it unlikely the above and other errors caught by debug compilations will be fixed any time soon. |
|
WCOSS2 (Dogwood) ctests Manually merge authoritative The A check of the wall times does not reveal any anomalous behavior. This is not a fatal fail. The Dogwood ctests results are consistent with @hu5970 Cactus results. That is, the changes in |
|
@RussTreadon-NOAA Thanks for testing PR on Dogwood. I will work with David to clean the code based on compiling information from using "DEBUG". After the clean, I will ask Guoqing and Xiaoyan to review the code again. If they do not have problem and the regression tests passed again. I will ask for merging the code. |
src/gsi/setupfed.f90
Outdated
| write(6,*) 'fed_highbnd=',fed_highbnd | ||
| write(6,*) 'maxval(ges_qg)=',maxval(ges_qg),'pe=',mype | ||
| ! write(6,*) 'maxval(geop_hgtl)=',maxval(geop_hgtl(:,:,:,it)) | ||
| write(6,*) 'maxval(ges_tsen)=',maxval(ges_tsen(:,:,:,it)) |
There was a problem hiding this comment.
@daviddowellNOAA @hu5970 Please delete or commented out the below two lines:
write(6,) 'maxval(ges_tsen)=',maxval(ges_tsen(:,:,:,it))
write(6,) 'ges_prsi',ges_prsi(100,100,1,1),ges_prsi(100,100,nsig,1)
The ges_tsen will crash GSI in the debug mode. Thanks.
Clean read_fed.f90 and setupfed.f90
|
Here are my comments copied from pull request #1 from @hu5970, which is now merged into the current PR. Thanks @hu5970. The proposed changes look good. The modifications to read_fed.f90 remove unused variables and also add a helpful check for valid lat-lon values. The changes to setupfed.f90 remove unused variables and also remove lines that were used for testing purposes but aren't actually needed for the new functionality. I compiled GSI with these changes and ran a test case of the GSI observer that includes FED observations. The results are identical to those from runs of the test case with GSI before the current changes in this PR. |
|
The regression test on WCOSS2: The reason of "hwrf_nmm_d3" failure is "The case has Failed the scalability test". It is not a fatal failure. |
|
The regression test results on Hera: The test results are on: |
Description
Initialization of the operational RRFSv1 will include assimilation of flash-extent density (FED) observations from the GOES Geostationary Lightning Mapper (GLM). The current PR is the first of at least 3 that will be needed to introduce the capability of FED assimilation into the code and regional workflow. The new capabilities that are added to GSI are:
Much of the code was originally developed by Rong Kong at OU-CAPS (Kong et al. 2020, Wang et al. 2021, Kong et al. 2022; https://doi.org/10.1175/MWR-D-19-0192.1, https://doi.org/10.1175/MWR-D-20-0406.1, https://doi.org/10.1175/MWR-D-21-0326.1). Recently, the observation operator has been modified by Amanda Back and Ashley Sebok based on tests with regional, convection-allowing FV3 forecasts. The new observation operator includes a cap of 8 flashes / minute for both the observed and simulated FED.
The observation operator is specific to the 3-km regional FV3 application in RRFS. Development of a more general observation operator is left to future work.
Fixes #588
Type of change
Please delete options that are not relevant.
How Has This Been Tested?
Initial tests were with NOAA-EMC GSI-EnKF code obtained in April 2023 and modified to include the assimilation of FED observations. A prototype of RRFSv1 was cycled hourly for 2.5 days, and the EnKF assimilation included FED data assimilation.
For the current PR, only the GSI observer with FED (and radar reflectivity) observations was tested. It produces identical results to those obtained in April 2023.
Checklist
DUE DATE for this PR is 8/24/2023. If this PR is not merged into
developby this date, the PR will be closed and returned to the developer.